Relax version verification #44

caioaamaral · 2021-03-21T22:54:03Z

Signed-off-by: Caio Amaral [email protected]

🦟 Bug fix

Fixes #36

Summary

Relax the --force-version verification method. Now it's not necessary to specify the fully-qualified version, instead if only the major version is passed, commands will be tested against the major version only.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

caioaamaral · 2021-03-21T22:58:20Z

I confess I've got a bit lost on how to write a test for this fix (neither figure out where are the tests), but if desired I can write one.

caioaamaral · 2021-03-30T10:50:13Z

@caguero friendly ping

caioaamaral · 2021-04-05T13:16:21Z

@chapulina @caguero friendly ping

caguero · 2021-04-05T19:38:00Z

@caioaamaral , I'll review your PR this week. Thanks for your contribution!

caguero

I'm a bit hesitant to change the behavior of this option. Specially because we might have situations where two major versions are installed and we want to choose one of them. If we apply this patch we'll lose the ability to do it.

~~I'd be open to add a new option --force-major-version that ignores the patch number.~~ See my next comment instead.

caguero

Could you also check the discussion on issue #36 please? There's a good suggestion by @chapulina for keeping just the --force-version argument but allowing multiple values:

Just a major version. E.g.: --force-version 9
Major and minor. E.g.: --force-version 9.1
Full version (current behavior). E.g.: --force-version 9.1.0

caioaamaral · 2021-04-05T21:15:45Z

Could you also check the discussion on issue #36 please? There's a good suggestion by @chapulina for keeping just the --force-version argument but allowing multiple values:
* Just a major version. E.g.: `--force-version 9`

* Major and minor. E.g.: `--force-version 9.1`

* Full version (current behavior). E.g.: `--force-version 9.1.0`

This change is expected to behave just like this. I'm splitting the version used as input into different fields (based on the number of '.'), then just those fields will be used when checking against the library version.

Ex:

required_version = '9.1'
library version = '9.1.3'

will check only two first fields of version, if they matches then this is the one that will be used.

This is achieved by this change on the current implementation:

    required_version = required_version.split('.')
    version = version.split('.')
    next unless required_version.zip(version).map {|required, current| required == current}.all?

if required_version has different size than version, required_version.zip(version) will have required_version size, ignoring the last fields from version

caguero · 2021-04-06T19:57:03Z

Could you also check the discussion on issue #36 please? There's a good suggestion by @chapulina for keeping just the --force-version argument but allowing multiple values:
* Just a major version. E.g.: `--force-version 9`

* Major and minor. E.g.: `--force-version 9.1`

* Full version (current behavior). E.g.: `--force-version 9.1.0`
This change is expected to behave just like this. I'm splitting the version used as input into different fields (based on the number of '.'), then just those fields will be used when checking against the library version.

Ex:
required_version = '9.1'
library version = '9.1.3'
will check only two first fields of version, if they matches then this is the one that will be used.

This is achieved by this change on the current implementation:
    required_version = required_version.split('.')
    version = version.split('.')
    next unless required_version.zip(version).map {|required, current| required == current}.all?
if required_version has different size than version, required_version.zip(version) will have required_version size, ignoring the last fields from version

Thanks for the clarification!

caguero

I did a quick experiment:

caguero@frost:~/workspace/ign-tools/build$ ign topic -l --force-version 9
Traceback (most recent call last):
	3: from /usr/bin/ign:236:in `<main>'
	2: from /usr/bin/ign:236:in `each_with_index'
	1: from /usr/bin/ign:236:in `each'
/usr/bin/ign:238:in `block in <main>': undefined method `split' for #<Array:0x00005576a168c2d0> (NoMethodError)

caguero@frost:~/workspace/ign-tools/build$ ign topic -l --force-version 9.1
Traceback (most recent call last):
	3: from /usr/bin/ign:236:in `<main>'
	2: from /usr/bin/ign:236:in `each_with_index'
	1: from /usr/bin/ign:236:in `each'
/usr/bin/ign:238:in `block in <main>': undefined method `split' for #<Array:0x00005558fd413a60> (NoMethodError)

ign topic -l --force-version 9.1.0
Traceback (most recent call last):
	3: from /usr/bin/ign:236:in `<main>'
	2: from /usr/bin/ign:236:in `each_with_index'
	1: from /usr/bin/ign:236:in `each'
/usr/bin/ign:238:in `block in <main>': undefined method `split' for #<Array:0x00005592b8143ef8> (NoMethodError)

Do you get the same error?

caioaamaral · 2021-04-06T23:38:36Z

Do you get the same error?

My mistake, sorry for the noise. Just forced push edd8b16 which will solve this problem.

Running locally using gazebo command (please ignore the XDG warning, gazebo opened just fine):
Edit: gazebo version is 3.8.0

orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.1
Version error: I cannot find this command in version [3.1].
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8.0
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8.1
Version error: I cannot find this command in version [3.8.1].

caguero

Thanks for the changes! It works for me as expected when I provide a version that matches my installed library. However it produces an exception when the library is not found, as opposed to show the error message "I cannot find the library...". E.g.:

ign topic -l --force-version 12
Traceback (most recent call last):
	3: from /usr/bin/ign:236:in `<main>'
	2: from /usr/bin/ign:236:in `each_with_index'
	1: from /usr/bin/ign:236:in `each'
/usr/bin/ign:237:in `block in <main>': undefined method `split' for ["12"]:Array (NoMethodError)

ign topic -l --force-version 12.1.2
Traceback (most recent call last):
	3: from /usr/bin/ign:236:in `<main>'
	2: from /usr/bin/ign:236:in `each_with_index'
	1: from /usr/bin/ign:236:in `each'
/usr/bin/ign:237:in `block in <main>': undefined method `split' for ["12", "1", "2"]:Array (NoMethodError)

caioaamaral · 2021-04-08T17:24:58Z

Thanks for the changes! It works for me as expected when I provide a version that matches my installed library. However it produces an exception when the library is not found, as opposed to show the error message "I cannot find the library...". E.g.:

Strange, I can't reproduce this error:

orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --versions
8.2.0
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --force-version 12
Version error: I cannot find this command in version [12].
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --force-version 12.1.2
Version error: I cannot find this command in version [12.1.2].

Maybe your binary is missing my last update?

caguero · 2021-04-08T18:37:06Z

Strange, I can't reproduce this error:

I think I know what's happening. My guess is that you only have one version of Ignition Transport installed. The problem seems to be here:

commands[ARGV[0]].each_with_index do |version, index|
    required_version = required_version.split('.')

Originally, required_version is a String and you can call split(). However you're overwriting required_version with the result of the split(), converting it to an Array. The next time you iterate in the loop (only if you have more than one version), split fails because it's not declared for Array types.

My suggestion is to use a separate local variable for the result of split(). As a bonus point, you'll also avoid to call join() later in case you need to print the error message.

caioaamaral · 2021-04-08T20:24:45Z

Strange, I can't reproduce this error:

I think I know what's happening. My guess is that you only have one version of Ignition Transport installed. The problem seems to be here:
commands[ARGV[0]].each_with_index do |version, index|
    required_version = required_version.split('.')
Originally, required_version is a String and you can call split(). However you're overwriting required_version with the result of the split(), converting it to an Array. The next time you iterate in the loop (only if you have more than one version), split fails because it's not declared for Array types.

My suggestion is to use a separate local variable for the result of split(). As a bonus point, you'll also avoid to call join() later in case you need to print the error message.

Got it, thanks for the clarification.
Edit: e473b66 applies your suggestion

Signed-off-by: Caio Amaral <[email protected]>

Signed-off-by: Carlos Agüero <[email protected]>

caguero

It works for me! I took the liberty of moving one line outside of the loop. Thanks for the patch!

Signed-off-by: Carlos Agüero <[email protected]>

…ools into fix/version_validation

caguero · 2021-04-09T20:48:05Z

I confess I've got a bit lost on how to write a test for this fix (neither figure out where are the tests), but if desired I can write one.

It's OK, for now we only have a static code checker test. There was a minor issue fixed in ce44884.

caioaamaral requested a review from caguero as a code owner March 21, 2021 22:54

github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Mar 21, 2021

caioaamaral mentioned this pull request Mar 21, 2021

Launch file may start wrong ign-gazebo version gazebosim/ros_gz#133

Closed

caioaamaral mentioned this pull request Mar 26, 2021

[ros2] Add default ign_gazebo version gazebosim/ros_gz#145

Closed

8 tasks

caguero requested changes Apr 5, 2021

View reviewed changes

caguero reviewed Apr 5, 2021

View reviewed changes

caguero requested changes Apr 6, 2021

View reviewed changes

caioaamaral force-pushed the fix/version_validation branch from 116e8bc to edd8b16 Compare April 6, 2021 23:33

caguero requested changes Apr 8, 2021

View reviewed changes

Relax version verification

e473b66

Signed-off-by: Caio Amaral <[email protected]>

caioaamaral force-pushed the fix/version_validation branch from edd8b16 to e473b66 Compare April 8, 2021 20:33

Move line outside of the loop

cee72e1

Signed-off-by: Carlos Agüero <[email protected]>

caguero approved these changes Apr 9, 2021

View reviewed changes

caguero and others added 3 commits April 9, 2021 22:10

Merge branch 'ign-tools1' into fix/version_validation

2b3e095

Style

ce44884

Signed-off-by: Carlos Agüero <[email protected]>

Merge branch 'fix/version_validation' of github.com:caioaamaral/ign-t…

f8bd3a9

…ools into fix/version_validation

caguero merged commit 1925384 into gazebosim:ign-tools1 Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax version verification #44

Relax version verification #44

caioaamaral commented Mar 21, 2021

caioaamaral commented Mar 21, 2021

caioaamaral commented Mar 30, 2021

caioaamaral commented Apr 5, 2021

caguero commented Apr 5, 2021

caguero left a comment •

edited

Loading

caguero left a comment

caioaamaral commented Apr 5, 2021 •

edited

Loading

caguero commented Apr 6, 2021 •

edited

Loading

caguero left a comment

caioaamaral commented Apr 6, 2021 •

edited

Loading

caguero left a comment

caioaamaral commented Apr 8, 2021

caguero commented Apr 8, 2021 •

edited

Loading

caioaamaral commented Apr 8, 2021 •

edited

Loading

caguero left a comment

caguero commented Apr 9, 2021

Relax version verification #44

Relax version verification #44

Conversation

caioaamaral commented Mar 21, 2021

🦟 Bug fix

Summary

Checklist

caioaamaral commented Mar 21, 2021

caioaamaral commented Mar 30, 2021

caioaamaral commented Apr 5, 2021

caguero commented Apr 5, 2021

caguero left a comment • edited Loading

Choose a reason for hiding this comment

caguero left a comment

Choose a reason for hiding this comment

caioaamaral commented Apr 5, 2021 • edited Loading

caguero commented Apr 6, 2021 • edited Loading

caguero left a comment

Choose a reason for hiding this comment

caioaamaral commented Apr 6, 2021 • edited Loading

caguero left a comment

Choose a reason for hiding this comment

caioaamaral commented Apr 8, 2021

caguero commented Apr 8, 2021 • edited Loading

caioaamaral commented Apr 8, 2021 • edited Loading

caguero left a comment

Choose a reason for hiding this comment

caguero commented Apr 9, 2021

caguero left a comment •

edited

Loading

caioaamaral commented Apr 5, 2021 •

edited

Loading

caguero commented Apr 6, 2021 •

edited

Loading

caioaamaral commented Apr 6, 2021 •

edited

Loading

caguero commented Apr 8, 2021 •

edited

Loading

caioaamaral commented Apr 8, 2021 •

edited

Loading